Skip to content

Fix performance issues #1122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 27, 2017
Merged

Fix performance issues #1122

merged 1 commit into from
Oct 27, 2017

Conversation

@joshsmith
Copy link
Contributor

@joshsmith joshsmith commented Oct 26, 2017 •
edited
Loading

What's in this PR?

Some fixes for API performance issues.

  • Remove preloads from views and write explicitly in controllers
  • Reduce id filter repetition
  • Ensure id query filters are run on every controller that has an index action
  • Add missing indexes

@joshsmith joshsmith force-pushed the fix-performance-issues branch from 3cbd781 to 5dc0bd9 Compare October 26, 2017 02:07
begedin
begedin reviewed Oct 26, 2017
View reviewed changes
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great change overall.

I especially like the move of preloads to controllers. It's more explicit and something that's generally recommended in phoenix.

Just a potential concern with removing tasks from a project, but should be fine otherwise.

test/lib/code_corps_web/views/project_view_test.exs
"type" => "task"
}
]
}
Copy link
Contributor

@begedin begedin Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check how this works with ember. Do we access project.tasks on the client end anywhere? I seem to remember this missing causing problems for us, but the problem may have gone away due to introducing task lists.

Copy link
Contributor Author

@joshsmith joshsmith Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is breaking things in Ember. Even worse, I wanted to be able to specify the tasks index to load only open tasks by default, but this appears to break functionality if the tasks are not included on the project.

There's some work to be done here figuring out the best way forward.

@joshsmith
Copy link
Contributor Author

joshsmith commented Oct 26, 2017

Do you agree with breaking out the preloads into the model @begedin?

@begedin
Copy link
Contributor

begedin commented Oct 26, 2017

@joshsmith If you mean, into the controller, then yeah, I absolutely do. Can't find a link for it, but pretty sure that's the recommended approach for phoenix

@begedin
Copy link
Contributor

begedin commented Oct 26, 2017 •
edited
Loading

To update myself, as I said on slack, nut sure the model module is the best place to put them, but we need them and I don't have a better idea, so I'm fine with placing them there.

Seconds later we agreed the view is a good spot.

@joshsmith joshsmith force-pushed the fix-performance-issues branch 2 times, most recently from 74c0fd7 to 393f3a1 Compare October 27, 2017 05:08
@joshsmith
Copy link
Contributor Author

joshsmith commented Oct 27, 2017 •
edited
Loading

AuthToken

  • value

Category

  • slug

Comment

  • user
  • task
  • github_id

DonationGoal:

  • current (unique to project)
  • project_id

GithubAppInstallation

  • github_id (unique)
  • user_id
  • project_id

GithubComment

  • github_id (unique)
  • github_issue_id

GithubEvent

  • github_delivery_id
  • status

GithubIssue

  • github_id (unique)
  • github_repo_id
  • github_pull_request_id

GithubPullRequest

  • github_id
  • github_repo_id

GithubRepo

  • github_id (unique)
  • github_app_installation_id (used by InstallationRepositories event but not sure it should be)

OrganizationGithubAppInstallation

  • organization_id
  • github_app_installation_id

These should probably be made a unique pair?

OrgnanizationInvite

  • code (unique)
  • email

Organization

  • approved
  • owner_id
  • slug (unique)

Preview

  • user_id

Project has:

  • slug (unique)
  • approved
  • organization_id

This has a github_repo and github_owner – what are these?

StripeX:

  • id_from_stripe

StripeConnectSubscription, StripePlatformCustomer

  • user_id

StripeExternalAccount

  • id_from_stripe (unique)

StripeFileUpload

  • id_from_stripe (unique)

Task

  • github_issue_id
  • github_repo_id
  • project_id
  • task_list_id
  • number
  • order
  • archived
  • status

TaskList has:

  • inbox
  • project_id
  • order

User:

  • github_id (unique)
  • email (unique)

@joshsmith
Copy link
Contributor Author

joshsmith commented Oct 27, 2017

Additionally, it doesn't look like we sort tasks by order, which is maybe a lost opportunity since that's expected on the front-end.

@joshsmith
Copy link
Contributor Author

joshsmith commented Oct 27, 2017

Should OrganizationGithubAppInstallation have a unique pair of organization_id and github_app_installation_id?

Project has a github_repo and github_owner – what are these?

@joshsmith
Add various performance fixes for API endpoints
bd93e86 
- Remove preloads from views and write explicitly in controllers
- Reduce id filter repetition
- Ensure id query filters are run on every controller that has an index action
- Add missing indexes
@joshsmith joshsmith force-pushed the fix-performance-issues branch from e820487 to bd93e86 Compare October 27, 2017 06:26
@begedin
Copy link
Contributor

begedin commented Oct 27, 2017 •
edited
Loading

Should OrganizationGithubAppInstallation have a unique pair of organization_id and github_app_installation_id?

I think no. Consider this case.

On my github account, I have 2 repos, intended to be two separate projects in two organizations. I install the CC app once and then from each project's installation tab, can see that installation and connect it to that organization.

It's a rare case, but I think it is possible.

Project has a github_repo and github_owner – what are these?

Probably remainder from before extracting project_github_repo or possibly even github_repo. We may have added them way early in this milestone.

@joshsmith
Copy link
Contributor Author

joshsmith commented Oct 27, 2017

Probably remainder from before extracting project_github_repo or possibly even github_repo. We may have added them way early in this milestone.

Can you add an issue for this?

begedin
begedin approved these changes Oct 27, 2017
View reviewed changes
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I see you decided to keep preloads in the controller. Works for me and makes sense, considering that's where they are used from.

@joshsmith joshsmith merged commit 5b2b3f3 into develop Oct 27, 2017
@joshsmith joshsmith deleted the fix-performance-issues branch October 27, 2017 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@begedin begedin begedin approved these changes

Assignees

No one assigned

Labels

needs code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joshsmith @begedin